-
Notifications
You must be signed in to change notification settings - Fork 639
🌱 autoscaling: try to gather the version information from MachineSets and e2e test improvements #5774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 autoscaling: try to gather the version information from MachineSets and e2e test improvements #5774
Conversation
|
/test pull-cluster-api-provider-aws-e2e-blocking |
f4098f8 to
151d33e
Compare
|
/retitle 🌱 autoscaling: try to gather the version information from MachineSets and e2e test improvements /test pull-cluster-api-provider-aws-e2e |
|
/retest |
| func (r *AWSMachineTemplateReconciler) machineSetToAWSMachineTemplate(ctx context.Context, o client.Object) []ctrl.Request { | ||
| md, ok := o.(*clusterv1.MachineSet) | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| // Check if it references an AWSMachineTemplate | ||
| if md.Spec.Template.Spec.InfrastructureRef.Kind != awsMachineTemplateKind { | ||
| return nil | ||
| } | ||
|
|
||
| // Return reconcile request for the referenced AWSMachineTemplate | ||
| return []ctrl.Request{ | ||
| { | ||
| NamespacedName: client.ObjectKey{ | ||
| Namespace: md.Namespace, | ||
| Name: md.Spec.Template.Spec.InfrastructureRef.Name, | ||
| }, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (r *AWSMachineTemplateReconciler) kubeadmControlPlaneToAWSMachineTemplate(ctx context.Context, o client.Object) []ctrl.Request
func (r *AWSMachineTemplateReconciler) machineDeploymentToAWSMachineTemplate(ctx context.Context, o client.Object) []ctrl.Request
func (r *AWSMachineTemplateReconciler) machineSetToAWSMachineTemplate(ctx context.Context, o client.Object) []ctrl.Request
These three functions are very similar. Should we refactor them a bit, similar to how we handle objectToAWSMachineTemplate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to prioritize merging this if it comes back green, in the interest of getting the release out. But let's see what @chrischdi says
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho not worth the complexity compared to these simple functions.
damdo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of non blocking nits
/lgtm
|
LGTM label has been added. Git tree hash: d422856fc8bdd4edd2300244a6a9b0cd70be5549
|
151d33e to
f1ea53a
Compare
damdo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
TY
|
LGTM label has been added. Git tree hash: ce26c1c04448c4b7076ed93ae7fcfdea1eb1a2cd
|
richardcase
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @chrischdi
It would be good to move the predicate to a shared package somewhere but we don't need to hold the release up for that.
/approve
| &clusterv1.MachineDeployment{}, | ||
| handler.EnqueueRequestsFromMapFunc(r.machineDeploymentToAWSMachineTemplate), | ||
| // Only emit events for creation to reconcile in case the MachineDeployment got created after the AWSMachineTemplate was reconciled. | ||
| builder.WithPredicates(predicate.Funcs{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this predicate somewhere so its reusable? Especially as its used twice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it as var and used it here for now. I did not see a dedicated predicates package as we have in CAPI and currently we only need it here.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: richardcase The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
f1ea53a to
06f3c98
Compare
…nd e2e test improvements
06f3c98 to
2a454ad
Compare
damdo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
LGTM label has been added. Git tree hash: 499cfebd6966332847b3334b42bf7599c7388c46
|
|
/retest |
|
@chrischdi: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/test pull-cluster-api-provider-aws-e2e-blocking |
|
/test pull-cluster-api-provider-aws-e2e |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Checklist:
Release note: